Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow inset-text to take a block #1078

Merged
merged 2 commits into from
Sep 4, 2019
Merged

Allow inset-text to take a block #1078

merged 2 commits into from
Sep 4, 2019

Conversation

ChrisBAshton
Copy link
Contributor

@ChrisBAshton ChrisBAshton commented Aug 30, 2019 •
edited
Loading

What

Can now pass an arbitrary block to the inset-text component to
have content rendered inset. If both 'text' and a block are
provided, 'text' takes precedence (mutually exclusive).

Why

This is a compromise solution, rather than copying the inset-prompt
component from content-publisher (for use in collections-publisher),
for the reason that under current GOV.UK architecture the component
CSS would be added to the stylesheets for our end users (not just
the users using the publishing app). See:
#1077

Once merged, we will use this latest inset-text component and
manually recreate what the inset-prompt component looks like by
passing components to the inset-text component as a block.

Trello: https://trello.com/c/TcBpLRuC/48-add-inset-prompt-component-to-govukpublishingcomponents

Visual Changes

Screen Shot 2019-08-30 at 15 43 56

View Changes

https://govuk-publishing-compo-pr-1078.herokuapp.com/

@bevanloon bevanloon temporarily deployed to govuk-publishing-compo-pr-1078 August 30, 2019 14:43 Inactive
@ChrisBAshton ChrisBAshton force-pushed the inset-text__take-block branch from 1a97cae to 2f046dc Compare August 30, 2019 14:45
@bevanloon bevanloon temporarily deployed to govuk-publishing-compo-pr-1078 August 30, 2019 14:45 Inactive
@bevanloon bevanloon temporarily deployed to govuk-publishing-compo-pr-1078 August 30, 2019 14:46 Inactive
@ChrisBAshton ChrisBAshton mentioned this pull request Aug 30, 2019
Closed
leenagupte
leenagupte previously requested changes Sep 2, 2019
View reviewed changes
Copy link
Contributor

@leenagupte leenagupte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think allowing a generic block is a good idea. It doesn't allow for consistent styling, and we'd have to depend on every place that uses the component to test how it renders and that it's accessible. If we're going to that we might as well create a new component in the application so at least all of the code for the component and its tests are in the same place.

I think it would be better to limit what the block will accept, e.g. a title and a list of items, and then add some tests around that.

@andysellick
Copy link
Contributor

We do have other components that accept a block like this, and the guidance around that is that the contents should be styled separately from the component, ie. the component takes no responsibility for what it is passed. We should at least document that in this component.

@leenagupte
Copy link
Contributor

leenagupte commented Sep 2, 2019 •
edited
Loading

We do have other components that accept a block like this, and the guidance around that is that the contents should be styled separately from the component, ie. the component takes no responsibility for what it is passed. We should at least document that in this component.

@andysellick Do you have an example?

@andysellick
Copy link
Contributor

There's a few, although they don't seem to document what I was talking about 😞

@ChrisBAshton
Copy link
Contributor Author

ChrisBAshton commented Sep 2, 2019 •
edited
Loading

We do have other components that accept a block like this, and the guidance around that is that the contents should be styled separately from the component, ie. the component takes no responsibility for what it is passed. We should at least document that in this component.

Happy to add a clarifying note. Where should that be added, @andysellick - the description at the top of the YAML file? Or is there a way of adding a description inside the block example? Will add a description field to the example.

@ChrisBAshton ChrisBAshton force-pushed the inset-text__take-block branch from 7ad89cb to 818b391 Compare September 2, 2019 09:24
@bevanloon bevanloon temporarily deployed to govuk-publishing-compo-pr-1078 September 2, 2019 09:24 Inactive
@andysellick
Copy link
Contributor

@ChrisBAshton https://github.com/alphagov/govuk_publishing_components/blob/master/app/views/govuk_publishing_components/components/docs/step_by_step_nav.yml#L76

@ChrisBAshton ChrisBAshton force-pushed the inset-text__take-block branch from 818b391 to e3c7ad7 Compare September 3, 2019 09:28
@bevanloon bevanloon temporarily deployed to govuk-publishing-compo-pr-1078 September 3, 2019 09:28 Inactive
@ChrisBAshton ChrisBAshton force-pushed the inset-text__take-block branch from e3c7ad7 to 04a1a47 Compare September 3, 2019 09:31
@bevanloon bevanloon temporarily deployed to govuk-publishing-compo-pr-1078 September 3, 2019 09:31 Inactive
@ChrisBAshton
Allow inset-text to take a block
faffbec 
Can now pass an arbitrary block to the inset-text component to
have content rendered inset. If both 'text' and a block are
provided, 'text' takes precedence (mutually exclusive).

This is a compromise solution, rather than copying the inset-prompt
component from content-publisher (for use in collections-publisher),
for the reason that under current GOV.UK architecture the component
CSS would be added to the stylesheets for our end users (not just
the users using the publishing app). See:
#1077

Once merged, we will use this latest inset-text component and
manually recreate what the inset-prompt component looks like by
passing components to the inset-text component as a block.

Trello: https://trello.com/c/TcBpLRuC/48-add-inset-prompt-component-to-govukpublishingcomponents
@ChrisBAshton
Update CHANGELOG
ec86bca
@ChrisBAshton ChrisBAshton force-pushed the inset-text__take-block branch from 04a1a47 to ec86bca Compare September 3, 2019 09:36
@leenagupte leenagupte dismissed their stale review September 3, 2019 14:18

Out of date

@ChrisBAshton ChrisBAshton merged commit 0e2ad75 into master Sep 4, 2019
@ChrisBAshton ChrisBAshton deleted the inset-text__take-block branch September 4, 2019 08:37
alex-ju added a commit that referenced this pull request Sep 5, 2019
@alex-ju
Move #1078 to unreleased
56a0cae
alex-ju added a commit that referenced this pull request Sep 5, 2019
@alex-ju
Move #1078 to unreleased
755d880
JonathanHallam added a commit that referenced this pull request Sep 5, 2019
@JonathanHallam
Version 20.0.0
e65fec9 
* **BREAKING:** Remove the inverse flag for contents list component ([PR #1090](#1090))
    * Will stop any inverse flags on the contents list component from working
* Set all branded links to correct focus colour ([PR #1088](#1088))
* Fix components focus state spacing ([PR #1054](#1054))
* Allow inset-text to take a block ([PR #1078](#1078))
@JonathanHallam JonathanHallam mentioned this pull request Sep 5, 2019
Merged
JonathanHallam added a commit that referenced this pull request Sep 5, 2019
@JonathanHallam
Version 20.0.0
aaa65f3 
* **BREAKING:** Remove the inverse flag for contents list component ([PR #1090](#1090))
    * Will stop any inverse flags on the contents list component from working
* Set all branded links to correct focus colour ([PR #1088](#1088))
* Fix components focus state spacing ([PR #1054](#1054))
* Allow inset-text to take a block ([PR #1078](#1078))
JonathanHallam added a commit that referenced this pull request Sep 5, 2019
@JonathanHallam
Version 20.0.0
4f903e0 
* **BREAKING:** Remove the inverse flag for contents list component ([PR #1090](#1090))
    * Will stop any inverse flags on the contents list component from working
* Set all branded links to correct focus colour ([PR #1088](#1088))
* Fix components focus state spacing ([PR #1054](#1054))
* Allow inset-text to take a block ([PR #1078](#1078))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andysellick andysellick andysellick approved these changes

@leenagupte leenagupte Awaiting requested review from leenagupte

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ChrisBAshton @andysellick @leenagupte @bevanloon